Import history, bookmarks, cookies, and passwords from Brave #185

garrettr · 2018-06-21T23:46:23Z

Partially resolves brave/brave-browser#24.
Resolves brave/brave-browser#220.

Since Brave browser-laptop is also based on Chromium (via Electron/Muon), some (but not all) user data is stored in an identical or mostly identical manner as it is in Chrome/Chromium; therefore, the corresponding import methods are based on, and similar (but not identical) to those for ChromeImporter, including: ImportHistory, ImportCookies, ImportPasswords.

ImportBookmarks was completely rewritten because browser-laptop stores bookmarks in a location and with a data structure that are totally different from Chromium's. However, it still uses the same high-level approach as ChromeImporter::ImportBookmarks—an in-order traversal of the bookmarks tree, because that's what AddBookmarks requires in order to maintain the relative order of the bookmarks once imported.

Some work is intentionally being left for follow-up issues (which I will file soon), including:

Brave equivalent of ChromeProfileLock
BraveImporter::ImportFavicons. Importing favicons from Brave is more complicated than it was for Chrome because Brave stores favicons in the cache along with all the other web content.

This PR also fixes some minor issues in ChromeImporter that were discovered during the development process.

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.

Test Plan:

yarn start. Go to the Brave menu and choose Import Bookmarks and Settings. Choose a Brave profile to import. Check Bookmarks, History, Cookies and Passwords to verify that they were imported correctly.
yarn test brave_unit_tests. All BraveImporterTest.* should pass.

This PR has been successfully manually tested on macOS, Windows 10, and Linux (Ubuntu 16.04, Brave installed via Debian package).

There are known issues with importing cookies and passwords from Brave when installed on Linux via Snapcraft which will have to be addressed in a follow-up.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions

darkdh

overall looks good just few places require changes. I'm still looking at the BraveImporter::ImportBookmarks but in the meanwhile you can address the comments

darkdh · 2018-06-25T17:02:44Z

patches/chrome-browser-importer-importer_list.cc.patch

@@ -73,36 +75,61 @@ index 3bf47fa746e2a417f89201f3e3800d8639f2e323..122e9daf552834db4d41c77a32033230
 +  AddChromeToProfiles(profiles, chromium_profiles, chromium_user_data_folder,
 +                      brandChromium);
 +}
+
+void DetectBraveProfiles(std::vector<importer::SourceProfile>* profiles) {


I'm thinking maybe we can put DetectBraveProfiles, DetectChromeProfiles and AddChromeToProfiles into
chromium_src/chrome/browser/importer/importer_list.cc
The patch is kind of bloated now

darkdh · 2018-06-25T17:50:27Z

test/data/import/brave-browser-laptop/README

+
+    TEST_UDD=/path/to/test/user/data/dir
+    mkdir -p $TEST_UDD
+    CHROME_USER_DATA_DIR=$TEST_UDD /Applications/Brave.app/Contents/MacOS/Brave


The usual way we use is /Applications/Brave.app/Contents/MacOS/Brave --user-data-dir-name=$TEST_UDD in muon based Brave (https://github.com/brave/muon/blob/tor_browser_context/atom/common/options_switches.cc#L34) or there will be some collision for crash reporter

Good to know! I didn't know it was also a command line flag, will test again to verify that it works and update these docs.

@darkdh As I recall now, I did try to use --user-data-dir-name at first, but unfortunately it does not seem to work as intended. It causes Brave to start with a fresh profile, but that profile's data never gets saved in the specified user data directory. After quitting Brave, the specified user data directory is empty. This does not happen when I use CHROME_USER_DATA_DIR. Saving the temporary state to disk is critical for my use case, since I need to extract the relevant Brave state files to use for test data.

I see, that sounds like a bug in muon

@darkdh Upon further testing, it looks like --user-data-dir has the expected behavior, while --user-data-dir-name does not. --user-data-dir is a Chromium flag, I'm not sure but it seems --user-data-dir-name might have been introduced in Muon. Should I update the documentation to recommend --user-data-dir instead of CHROME_USER_DATA_DIR?

yes, please. thanks

darkdh · 2018-06-25T22:37:12Z

utility/importer/brave_importer.h

+#include "chrome/common/importer/imported_bookmark_entry.h"
+#include "chrome/utility/importer/importer.h"
+
+class BraveImporter : public Importer {


We should inherit ChromeImporter and make Import* virtual functions so that we don't have to duplicate codes for ImportPasswords and ImportCookies

I'm with you in spirit, and will give this a shot, but in my experience there are unfortunately some minor differences between how Chromium and Brave end up storing data which makes it tricky to simply reuse the code from ChromeImporter. However, I believe some of these are probably easy to resolve and others should be resolved in browser-laptop rather than worked around in brave-core. I will give this a shot and file follow-ups as I go.

darkdh · 2018-06-27T04:05:45Z

great work on ChromeImporter::ImportBookmarks 👍

darkdh · 2018-06-27T04:08:11Z

patches/chrome-browser-importer-external_process_importer_client.cc.patch

+
+ #include "base/bind.h"
+ #include "base/strings/string_number_conversions.h"
+#include "brave/grit/generated_resources.h"


Please rebase to master. We don't need to patch this now,
see 5b4838a

darkdh · 2018-06-27T04:09:13Z

patches/chrome-browser-importer-importer_list.cc.patch

 +#include "brave/common/importer/chrome_importer_utils.h"
+#include "brave/grit/generated_resources.h"


garrettr · 2018-07-05T23:01:06Z

@darkdh Ready for re-review

darkdh · 2018-07-05T23:10:15Z

lgtm, would you mind doing a rebase? it shows conflict against master.

…b4838a

garrettr · 2018-07-05T23:36:19Z

@darkdh Rebased!

darkdh · 2018-07-05T23:37:48Z

utility/importer/brave_importer.cc

+    }
+  }
+#else
+  base::FilePath prefs_path =


actually can we pass as argument? since it is the only deviation compared to ChromeImporter:: ImportPasswords so that we don't have to override it

darkdh · 2018-07-06T00:00:00Z

patches/chrome-browser-ui-content_settings-content_setting_image_model.cc.patch

@@ -1,5 +1,5 @@
 diff --git a/chrome/browser/ui/content_settings/content_setting_image_model.cc b/chrome/browser/ui/content_settings/content_setting_image_model.cc
-index 366ba05a508c8e5f77bd0a380ff229da73065819..b24ef19f81ab2c469780caab71a5953d95750d16 100644
+index 366ba05a508c8e5f77bd0a380ff229da73065819..4f159eee09eb691c796681af5e669df442aca7b7 100644


this patch seems irrelevant to this PR

darkdh · 2018-07-06T00:09:17Z

patches/chrome-utility-importer-profile_import_impl.cc.patch

@@ -19,7 +19,7 @@ index 1cbcc2fcb663c43daad9e6ad86f97e74b964aa11..2910707b7f569cb87947c81d8522f1d9
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
-@@ -39,6 +43,16 @@ void ProfileImportImpl::StartImport(
+@@ -39,6 +43,19 @@ void ProfileImportImpl::StartImport(


can we subclass ProfileImportImpl and only overrides StartImport?
In overridden method do OSCrypt password hack first and then call parent's StartImport
so we only need to substitute ProfileImportImpl to BraveProfileImportImpl by preprocessor in chrome/utility/importer/profile_import_service.cc with only one line

There is one unrelated request but it will allow us to remove this patch completely
can we use preprocessor for BraveExternalProcessImporterBridge ?

darkdh · 2018-07-06T00:15:10Z

patches/components-os_crypt-key_storage_keyring.cc.patch


   std::string password;
   gchar* password_c = nullptr;
 +  const char* application_name;
 +  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
 +  if (command_line->HasSwitch("import-chrome")) {
 +    application_name = "chrome";
-+  } else if (command_line->HasSwitch("import-chromium")) {
+  } else if (command_line->HasSwitch("import-chromium") ||


let's use preprocessor to get rid of this patch too

darkdh · 2018-07-06T00:17:12Z

patches/components-os_crypt-key_storage_kwallet.cc.patch

@@ -19,7 +19,8 @@ index aa4b3d4a6b5c7b1f725d6446ea5e573094ec935b..4c4c7191a57afc7a42dfa87701578bd9
 +  if (command_line->HasSwitch("import-chrome")) {
 +    folder_name = "Chrome Keys";
 +    key = "Chrome Safe Storage";
-+  } else if (command_line->HasSwitch("import-chromium")) {
+  } else if (command_line->HasSwitch("import-chromium") ||


darkdh · 2018-07-06T00:17:21Z

patches/components-os_crypt-key_storage_libsecret.cc.patch

@@ -28,7 +28,8 @@ index a1b5b975bc89c4891643adab17ce0b00ba2a8032..9d4be365463d2555101a96006ca4c68a
 +  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
 +  if (command_line->HasSwitch("import-chrome")) {
 +    application_name = "chrome";
-+  } else if (command_line->HasSwitch("import-chromium")) {
+  } else if (command_line->HasSwitch("import-chromium") ||


darkdh · 2018-07-06T00:17:50Z

patches/components-os_crypt-keychain_password_mac.mm.patch

@@ -26,7 +26,8 @@ index 2b38db266f9aa1f4141c8649c021042ede4e5589..4b14f15e5091b251be53eff29139beaa
 +  if (command_line->HasSwitch("import-chrome")) {
 +    service_name = "Chrome Safe Storage";
 +    account_name = "Chrome";
-+  } else if (command_line->HasSwitch("import-chromium")) {
+  } else if (command_line->HasSwitch("import-chromium") ||


darkdh

we can do the eliminating patches task in follow up

darkdh · 2018-07-10T20:16:17Z

@garrettr could you open an issue for follow up? Thanks.
I'm gonna merge this PR first.

bbondy · 2018-07-10T21:05:54Z

WHOOOOOHOOOO 🎉

Toggle Tweak & Alignment Tweak

garrettr requested a review from darkdh June 21, 2018 23:46

bbondy assigned garrettr Jun 25, 2018

garrettr mentioned this pull request Jun 25, 2018

Prompt user to close Brave before importing from a Brave profile brave/brave-browser#423

Closed

darkdh requested changes Jun 25, 2018

View reviewed changes

darkdh reviewed Jun 27, 2018

View reviewed changes

This was referenced Jun 27, 2018

Import favicons from Brave brave/brave-browser#440

Closed

Passwords are stored differently when Brave is installed via Snap vs. deb brave/browser-laptop#14586

Closed

garrettr force-pushed the import-brave branch from 4debd11 to 35d64c8 Compare June 28, 2018 22:43

Garrett Robinson added 7 commits July 5, 2018 16:20

Import history, bookmarks, cookies, and passwords from Brave

f2cac13

Fix importer process crash due to missing localized string

7def0b4

Remove includes made unnecessary by 5b4838a

10798a8

Fix crash in ExternalProcessImporterBridge::GetLocalizedString due to 5…

6ca065f

…b4838a

Recommend --user-data-dir instead of CHROME_USER_DATA_DIR

2509389

Minimize patching of importer_list.cc

994da20

Subclass ChromeImporter to reuse code where possible

6cd1669

garrettr force-pushed the import-brave branch from b7faf87 to 6cd1669 Compare July 5, 2018 23:36

darkdh reviewed Jul 5, 2018

View reviewed changes

darkdh reviewed Jul 6, 2018

View reviewed changes

Reuse ImportPasswords by adding prefs filename parameter

33d98a8

darkdh reviewed Jul 6, 2018

View reviewed changes

Revert unnecessary change to patchfile

dcd667c

darkdh approved these changes Jul 10, 2018

View reviewed changes

darkdh merged commit 35d6d67 into master Jul 10, 2018

garrettr mentioned this pull request Jul 10, 2018

Minimize patches for {Chrome,Brave}Importer brave/brave-browser#511

Closed

bbondy deleted the import-brave branch August 23, 2018 13:40

cezaraugusto added a commit that referenced this pull request Jul 17, 2019

Imported from brave-ui: Merge pull request #185 from brave/style-updates

f91bf15

Toggle Tweak & Alignment Tweak

bsclifton mentioned this pull request Mar 9, 2020

Import data and settings from another Brave channel brave/brave-browser#1357

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import history, bookmarks, cookies, and passwords from Brave #185

Import history, bookmarks, cookies, and passwords from Brave #185

garrettr commented Jun 21, 2018

darkdh left a comment

darkdh Jun 25, 2018

darkdh Jun 25, 2018

garrettr Jun 26, 2018

garrettr Jun 26, 2018

darkdh Jun 26, 2018

garrettr Jun 27, 2018

darkdh Jun 27, 2018

darkdh Jun 25, 2018

garrettr Jun 26, 2018

darkdh commented Jun 27, 2018

darkdh Jun 27, 2018

darkdh Jun 27, 2018

garrettr commented Jul 5, 2018

darkdh commented Jul 5, 2018

garrettr commented Jul 5, 2018

darkdh Jul 5, 2018

darkdh Jul 6, 2018

darkdh Jul 6, 2018

darkdh Jul 6, 2018

darkdh Jul 6, 2018

darkdh Jul 6, 2018

darkdh Jul 6, 2018

darkdh Jul 6, 2018

darkdh left a comment

darkdh commented Jul 10, 2018

bbondy commented Jul 10, 2018

		+#include "brave/common/importer/chrome_importer_utils.h"
		+#include "brave/grit/generated_resources.h"

Import history, bookmarks, cookies, and passwords from Brave #185

Import history, bookmarks, cookies, and passwords from Brave #185

Conversation

garrettr commented Jun 21, 2018

Submitter Checklist:

Test Plan:

Reviewer Checklist:

darkdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrettr commented Jul 5, 2018

darkdh commented Jul 5, 2018

garrettr commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

darkdh commented Jul 10, 2018

bbondy commented Jul 10, 2018